Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces cross-process locking for the stack state file to prevent concurrent write races, and standardizes CLI handling/documentation for lock-related failures (exit code 8).
Changes:
- Add a gitDir-scoped file lock (
gh-stack.lock) and makestack.Save()acquire/release it around writes. - Add
SaveLocked()for atomic Lock + Load + Modify + Save sequences and add new lock-focused tests. - Map lock acquisition failures to a dedicated CLI exit code (8) and document it in README/SKILL docs.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/gh-stack/SKILL.md | Document exit code 8 semantics (stack locked, 5s timeout). |
| README.md | Add exit code 8 to the public exit-code table. |
| internal/stack/stack.go | Make Save() lock-protected; add SaveLocked() and shared write helper. |
| internal/stack/lock.go | Implement lock acquisition/retry/timeout and lock lifecycle. |
| internal/stack/lock_unix.go | Unix flock implementation for tryLock/unlock. |
| internal/stack/lock_windows.go | Windows LockFileEx implementation for tryLock/unlock. |
| internal/stack/lock_test.go | Add tests validating mutual exclusion and concurrent write serialization. |
| cmd/utils.go | Add ErrLockFailed (exit 8) and handleSaveError() helper. |
| cmd/add.go | Use handleSaveError() when persisting stack changes. |
| cmd/init.go | Use handleSaveError() for save failures. |
| cmd/push.go | Use handleSaveError() for save failures. |
| cmd/sync.go | Use handleSaveError() for save failures (main save path). |
| cmd/unstack.go | Use handleSaveError() for save failures. |
| cmd/view.go | Keep best-effort save after PR sync (now under lock). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning This is an internal experiment to assess Copilot's ability to auto-approve PRs. Please 👍 this comment if the assessment below is correct and 👎 if not. Feedback in #f-ccr-auto-approve is appreciated! Copilot thinks this PR is not ready to approve — see review comments for details. |
There was a problem hiding this comment.
Pull request overview
Adds cross-process locking around .git/gh-stack persistence to reduce concurrency issues, introduces a dedicated exit code for lock contention, and updates commands/docs accordingly.
Changes:
- Introduces an advisory OS-level file lock (
gh-stack.lock) with timeout/retry logic and tests. - Updates stack persistence APIs (
Save,SaveNonBlocking,SaveLocked) and routes save failures through a sharedhandleSaveError(exit code 8 on lock contention). - Updates CLI commands and documentation to reflect the new lock-related exit code/behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/gh-stack/SKILL.md | Documents new exit code 8 and describes lock timeout behavior. |
| README.md | Adds exit code 8 to the public exit code list. |
| internal/stack/stack.go | Wraps saves with locking; adds non-blocking and “already locked” save variants. |
| internal/stack/lock.go | Implements lock acquisition/retry/timeout and lock error type. |
| internal/stack/lock_unix.go | Unix implementation of non-blocking exclusive advisory locks. |
| internal/stack/lock_windows.go | Windows implementation of non-blocking exclusive locks. |
| internal/stack/lock_test.go | Adds coverage for basic locking behavior and concurrent serialization. |
| cmd/utils.go | Adds ErrLockFailed (exit 8) and handleSaveError to standardize save error handling. |
| cmd/add.go | Uses handleSaveError when persisting stack changes. |
| cmd/init.go | Uses handleSaveError when persisting initial stack state. |
| cmd/push.go | Uses handleSaveError when persisting updated PR/base SHA state. |
| cmd/sync.go | Uses non-blocking save on conflict paths; uses handleSaveError on normal save path. |
| cmd/unstack.go | Uses handleSaveError for save failures. |
| cmd/merge.go | Switches PR-state persistence to non-blocking save. |
| cmd/rebase.go | Switches PR-state persistence to non-blocking save. |
| cmd/view.go | Switches PR-state persistence to non-blocking save. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning This is an internal experiment to assess Copilot's ability to auto-approve PRs. Please 👍 this comment if the assessment below is correct and 👎 if not. Feedback in #f-ccr-auto-approve is appreciated! Copilot thinks this PR is not ready to approve — see review comments for details. |
File Locking Implementation:
FileLocktype ininternal/stack/lock.gowith cross-platform support (lock_unix.go,lock_windows.go) to acquire an exclusive lock on the stack file during writes, preventing concurrent modification by multiple processes. Includes a 5-second timeout and robust unlock logic. [1] [2] [3]lock_test.goto verify locking behavior, including concurrent access and lock file persistence.Stack File Write Changes:
Saveininternal/stack/stack.goto acquire and release the lock around writes. AddedSaveLockedfor advanced use cases where the lock is held across multiple operations.Error Handling and User Feedback:
LockErrorand exit code for lock failures, updating all commands that save the stack file (add.go,init.go,push.go,sync.go,unstack.go) to use a newhandleSaveErrorhelper for clearer user messages when a lock cannot be acquired. [1] [2] [3] [4] [5] [6]README.mdandskills/gh-stack/SKILL.md. [1] [2] [3]Documentation and Comments:
Best-effort Save in View:
view.goto clarify that saving after syncing PR state is best-effort and should not block the user if locking fails.